Sequence equality #249

frenchy64 · 2025-02-07T06:35:32Z

Closes #243
Closes #250
Closes #251
Closes #274
Closes #283

It would be nice to replace nullptr returns with nil in sequenceable but it enables important optimizations for calls to sequenceable member functions. I did not investigate returning option types.

jeaye

Lots of good fixes in here. I think there are some introduced bugs though. I tried to pick them out.

compiler+runtime/src/cpp/jank/runtime/core.cpp

compiler+runtime/src/cpp/jank/runtime/obj/chunked_cons.cpp

compiler+runtime/src/cpp/jank/runtime/obj/cons.cpp

compiler+runtime/test/cpp/jank/runtime/core/seq.cpp

frenchy64 · 2025-02-23T07:54:05Z

Found and fixed a couple more issues while I scanned everything. Lmk if I should pull them out.

apply_to isn't compatible with seqs that return fresh seqs from next_in_place. Not sure if they exist, but persistent_list is implemented that way.
removed transient cases to resolve Transients mutate in-place on persistent ops #274
- extra: conj case for seqables didn't seem right to me compared to Clojure, so I removed it. For one, it changes the type of the seqable to a seq.
added paranoid assertions for nil while the function/template ambiguity exists for runtime::{next,seq,next_in_place,fresh_seq}.

jeaye

Mostly a rehash of our slack discussion, but also called out some other concerns.

jeaye · 2025-02-23T19:56:39Z

compiler+runtime/src/cpp/jank/runtime/behavior/callable.cpp

@@ -878,88 +878,88 @@ namespace jank::runtime
          case 1:
            return dynamic_call(source, s->first());
          case 2:
-            return dynamic_call(source, s->first(), s->next_in_place()->first());
+            return dynamic_call(source, s->first(), (s = s->next_in_place())->first());


I have a local TODO that notes that persistent_list_sequence shouldn't exist, since persistent_list itself is a sequence.

This change is a big red flag, since this is an extremely hot code path. I don't want to impact its perf for this one case which needs to change anyway.

If you want to add a comment here noting that it doesn't work for persistent_list, that's ok. If you want to remove persistent_list_sequence, that's also ok, but let's do it in a separate PR.

Either way, let's please undo this file's changes.

Either way, let's please undo this file's changes.

Sure.

I don't want to impact its perf for this one case which needs to change anyway.

I don't think the bug is reachable right now.

persistent_list::next_in_place is the potential problem, but it's currently unreachable to apply_to since a fresh seq is taken and then persistent_list_sequence::next_in_place is called instead.

I'm not sure what would happen if persistent_list_sequence were removed and (presumably) replaced with persistent_list itself. I think the apply_to bug would be introduced if persistent_list::next_in_place continued to allocate a new list, as is implied in this comment.

persistent_list_sequence_ptr persistent_list::next_in_place() const { /* In-place updates don't make sense for lists, since any call to fresh_seq would return * a list sequence. So we know, principally, that a list itself cannot be considered fresh. */ return next(); }

The comment would no longer apply, if we removed persistent_list_sequence. This would be updated to actually mutate the list.

compiler+runtime/include/cpp/jank/runtime/var.hpp

jeaye · 2025-02-23T19:59:33Z

compiler+runtime/src/cpp/jank/runtime/core.cpp

-    else if(!rhs)
+    if(!rhs)


Why remove the else? I find it helps with the logical flow. Also, by having the else if, your assertion is proven by the compiler to be redundant.

I added back the else. Since the compiler pointed out the assertion was redundant, does that mean returning false is equivalent to returning !lhs?

Yep, equivalent. Was just like that for symmetry.

jeaye · 2025-02-23T20:12:10Z

compiler+runtime/src/cpp/jank/runtime/obj/chunked_cons.cpp

+          assert(it != nil::nil_const());
+          assert(seq != nil::nil_const());


It's impossible for these assertions to be false, proven by the type system. I think you're concerned about the next_in_place(it) call somehow returning an object_ptr. But it could never do that, since it would result in a compiler error.

Now, the history is that next_in_place has been in flux for a year or so and I was making some changes which would allow for it to be optional. That never happened, but it's why we're not using it->next_in_place() here. Ultimately, it's doing the same thing and all of these can be changed back. I just haven't done that yet, since it's a perf change and I haven't been focusing on perf.

Either way, your concerns about this being nil should not be keeping you up. auto it(fresh_seq()); means that we'll have a fully typed it for the currently visited object. We cannot assign an object_ptr to it. C++ won't allow it.

Thanks for the explanation. What about something like this loop? Can this be nil?

jank/compiler+runtime/src/cpp/clojure/core_native.cpp

Lines 575 to 581 in bf75688

fn->arity_3 = [](object * const l, object * const r, object * const rest) -> object * {

if(!is_equiv(l, r))

{

return obj::boolean::false_const();

}

for(auto it(fresh_seq(rest)); it != nullptr; it = next_in_place(it))

Eep.

clojure.core=> (== 1 1 1 1 1) Exception: not a number: nil

Opened #283

Included fix here.

That one can be nil, yes, since it's using the object_ptr overloads. However, it cannot be nullptr, since those functions never return nullptr.

jeaye · 2025-02-23T20:17:50Z

compiler+runtime/src/cpp/jank/runtime/obj/repeat.cpp

@@ -16,6 +16,7 @@ namespace jank::runtime::obj
    : value{ value }
    , count{ count }
  {
+    assert(0 < to_int(count));


Feels like this could be an exception, rather than assertion, since someone embedding jank may create it a repeat with 0 and we don't need to kill the whole program.

Done.

It doesn't necessarily need to be an error either. We can made these constructors robust by adding checks in its members.

jeaye

All good with your previous changes. The new bug fix has a comment, though. I'll fix it locally.

Thanks for the iteration, Ambrose!

jeaye · 2025-02-24T00:49:30Z

compiler+runtime/src/cpp/clojure/core_native.cpp

@@ -578,7 +578,8 @@ jank_object_ptr jank_load_clojure_core_native()
        return obj::boolean::false_const();
      }

-      for(auto it(fresh_seq(rest)); it != nullptr; it = next_in_place(it))
+      for(auto it(fresh_seq(rest)); it != nullptr && it != obj::nil::nil_const();


So, here, it cannot be nullptr. That's not part of the contract. Everything returned from runtime:: fns should be non-null.

The one exception is the next_in_place and fresh_seq templates. Ultimately, we can remove those (not in this PR); they're only there for a historical partial refactor which never panned out.

There should never be a case, right now, where we need to check for nullptr and nil.

There should never be a case, right now, where we need to check for nullptr and nil.

There's a few more in multi_function.cpp.

frenchy64 added 15 commits February 7, 2025 00:34

wip range equality

1fc35d5

wip

bbc06d0

test core::equal

a059c68

wip

701d79f

[skip ci]

8a1472e

[skip ci] rm

cbd354f

[skip ci]

3727186

tests

ddd82ac

wip

c2da550

[skip ci]

c035ee8

[skip ci]

4dc1d92

wip

b6d8889

wip

408ed65

fix

4529d26

[skip ci]

c227006

frenchy64 changed the title ~~[wip] range equality~~ [wip] sequence equality Feb 7, 2025

frenchy64 added 14 commits February 9, 2025 21:58

fix

f1d6e2c

[skip ci]

ca229f8

wip

3afba55

[skip ci]

c5f2e33

fmt

2dbbca6

Merge branch 'main' into sequence-equality

d081d74

Merge branch 'main' into sequence-equality

4819593

assert

fe6f1dd

Merge branch 'main' into sequence-equality

2deb82d

[skip ci]

c36ca8d

wip

acb06a4

[skip ci]

4a1620f

[skip ci]

b3ffc63

[skip ci] undo

8bf2c34

jeaye requested changes Feb 22, 2025

View reviewed changes

frenchy64 added 12 commits February 22, 2025 13:45

[skip ci] revert equal null check

674a8e9

[skip ci] fix first half of chunked cons

d754b74

[skip ci] add bang to next-in-place, fix comments

6132099

[skip ci] assert

7c16d3a

asserts

d9d6281

[skip ci] -else

e562511

add sequence_equal tests

0d27f83

apply-to

64a74a2

[skip ci]

eee2c29

tests

cea8422

fmt

2e08987

wip

35a8643

frenchy64 requested a review from jeaye February 23, 2025 07:54

jeaye requested changes Feb 23, 2025

View reviewed changes

frenchy64 added 11 commits February 23, 2025 16:40

[skip ci] undo equal changes

5aecb6e

[skip ci] revert callable

0da1002

[skip ci] rm assertions

5df808d

[skip ci] remove assert

bf75688

fix jank-lang#283

201560b

[skip ci] smaller fix

f2e9a29

[skip ci] try again

4b405bf

[skip ci] again

4246bd8

[skip ci] -assert

24ca1fc

[skip ci] use correct ctor for infinite repeat

aaacb2c

fmt

9f05135

frenchy64 requested a review from jeaye February 24, 2025 00:39

jeaye approved these changes Feb 24, 2025

View reviewed changes

jeaye merged commit 6371c32 into jank-lang:main Feb 24, 2025
7 checks passed

		assert(it != nil::nil_const());
		assert(seq != nil::nil_const());

	fn->arity_3 = [](object * const l, object * const r, object * const rest) -> object * {
	if(!is_equiv(l, r))
	{
	return obj::boolean::false_const();
	}

	for(auto it(fresh_seq(rest)); it != nullptr; it = next_in_place(it))

Uh oh!

Sequence equality #249

Sequence equality #249

Uh oh!

Conversation

frenchy64 commented Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeaye left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

frenchy64 commented Feb 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeaye left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeaye Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frenchy64 Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeaye left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

frenchy64 commented Feb 7, 2025 •

edited

Loading

frenchy64 commented Feb 23, 2025 •

edited

Loading

jeaye Feb 24, 2025 •

edited

Loading

frenchy64 Feb 24, 2025 •

edited

Loading